fix: standardize API token format across all creation paths#44
fix: standardize API token format across all creation paths#44wicky-zipstack wants to merge 3 commits intomainfrom
Conversation
Three places were creating API tokens with inconsistent formats: - user.py: used uuid4().hex (no vtk_ prefix, no label, no signature) - views.py: stored raw frontend token (no format enforcement) - api_tokens/views.py: generated vtk_ but never persisted to DB All now use generate_api_key() for consistent vtk_ prefix, with label "Default", proper signature, and configurable expiry via API_KEY_EXPIRY_DAYS.
wicky-zipstack
requested review from
Deepak-Gnanasekar,
abhizipstack and
tahierhussain
|
| Filename | Overview |
|---|---|
| backend/backend/core/routers/api_tokens/views.py | Legacy generate_token upgraded to upsert-based DB record creation with audit log and token_hash — but else branch bypasses MAX_KEYS_PER_USER |
| backend/backend/core/views.py | Token lookup scoped to label='Default', fresh vtk_ key generated, audit log added — but create path bypasses MAX_KEYS_PER_USER |
| backend/backend/core/user.py | Replaces uuid4().hex with generate_api_key(), adds label, signature, and API_KEY_EXPIRY_DAYS — clean and consistent |
Sequence Diagram
sequenceDiagram
participant FE as Frontend
participant VP as views.py<br/>(update_user_token)
participant UP as user.py<br/>(get_or_create_valid_token)
participant LG as api_tokens/views.py<br/>(generate_token)
participant DB as APIToken DB
participant AUD as Audit Log
FE->>VP: PUT /profile {token: "vtk_..."}
VP->>DB: filter(user, label='Default').first()
DB-->>VP: existing | None
alt token unchanged
VP-->>FE: return early
else token changed
VP->>DB: existing.delete()
note over VP,DB: ⚠ No MAX_KEYS check
VP->>DB: create(vtk_, sig, label='Default', expiry)
VP->>AUD: log(action='create')
VP-->>FE: 200 OK
end
Note over UP: Called during auth/session init
UP->>DB: filter(user, org).select_for_update()
DB-->>UP: token | None
alt token invalid/expired
UP->>DB: token.delete()
end
UP->>DB: create(vtk_, sig, label='Default', expiry)
FE->>LG: POST /token/generate
LG->>DB: filter(user, label='Default').first()
DB-->>LG: existing | None
alt existing Default token
LG->>DB: existing.save(update_fields=[token,token_hash,sig,expiry])
LG->>AUD: log(action='regenerate')
else no Default token
note over LG,DB: ⚠ No MAX_KEYS check
LG->>DB: create(vtk_, sig, label='Default', expiry)
LG->>AUD: log(action='create')
end
LG-->>FE: {token: "vtk_..."}
Prompt To Fix All With AI
This is a comment left during a code review.
Path: backend/backend/core/routers/api_tokens/views.py
Line: 211-219
Comment:
**`MAX_KEYS_PER_USER` limit not enforced on new-token creation path**
The upsert logic correctly updates an existing `"Default"` token in-place, but when no `"Default"` token exists the `else` branch creates a brand-new `APIToken` record without first checking `MAX_KEYS_PER_USER`. A user who already holds the maximum number of non-Default keys can call this legacy endpoint to silently create an extra token that exceeds the per-user cap enforced by `create_api_key`.
`create_api_key` (lines 66–72 of the same file) shows the guard pattern:
```python
max_keys = django_settings.MAX_KEYS_PER_USER
existing_count = APIToken.objects.filter(user=request.user).count()
if existing_count >= max_keys:
return Response({"message": f"Maximum {max_keys} API keys allowed per user."}, status=status.HTTP_429_TOO_MANY_REQUESTS)
```
The same check should be added to `generate_token` before the `else` branch creates a new record.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: backend/backend/core/views.py
Line: 62-80
Comment:
**`MAX_KEYS_PER_USER` limit not enforced in `update_user_token`**
Before this PR `update_user_token` stored the raw frontend string — no real DB row was inserted. Now it calls `APIToken.objects.create(...)` (line 70), creating a real row that counts against the per-user key cap. However, no `MAX_KEYS_PER_USER` guard is applied before the create call, unlike `create_api_key`. A user who has reached the maximum via the KeyManagement UI can get an extra token simply by saving their profile page.
Consider adding the same count check:
```python
max_keys = django_settings.MAX_KEYS_PER_USER
if APIToken.objects.filter(user=user).count() >= max_keys:
return # or surface an appropriate error
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (3): Last reviewed commit: "fix: address Greptile round 2 — scope fi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| to maintain consistency with the api-keys/create endpoint. | ||
| """ | ||
| # Delete any existing default token for this user | ||
| APIToken.objects.filter(user=request.user, label="Default").delete() |
There was a problem hiding this comment.
APIToken.objects.filter(user=request.user, label="Default").delete() removes every token labeled "Default" for the user — not only ones generated by this legacy endpoint. A user who created a key via the KeyManagement UI and chose to name it "Default" will have it silently destroyed the next time this endpoint is called.
Consider a more precise scoping strategy, for example:
- Adding a
sourcefield toAPITokento distinguish auto-generated vs. user-managed keys, or - Switching to an upsert pattern (update token value in place) rather than delete-then-create, or
- At minimum, limiting deletion to a single record (
[:1].delete()) to reduce blast radius.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/backend/core/routers/api_tokens/views.py
Line: 191
Comment:
**Deletes ALL 'Default'-labeled tokens, including user-managed ones**
`APIToken.objects.filter(user=request.user, label="Default").delete()` removes every token labeled `"Default"` for the user — not only ones generated by this legacy endpoint. A user who created a key via the KeyManagement UI and chose to name it `"Default"` will have it silently destroyed the next time this endpoint is called.
Consider a more precise scoping strategy, for example:
- Adding a `source` field to `APIToken` to distinguish auto-generated vs. user-managed keys, or
- Switching to an upsert pattern (update token value in place) rather than delete-then-create, or
- At minimum, limiting deletion to a single record (`[:1].delete()`) to reduce blast radius.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed — replaced the DELETE ... label='Default' with an upsert pattern. Now finds the existing auto-generated token and updates it in place, instead of deleting all matching tokens. Also added log_api_key_event() audit call.
- P1: Replace delete-all-by-label with upsert in generate_token to avoid destroying user-created keys named "Default" - P2: Add log_api_key_event() to generate_token and update_user_token - P2: Rename new_token to token_value with comment clarifying it is only used as an "unchanged" gate, not stored
- P1: Scope update_user_token filter to label="Default" to avoid deleting user-managed keys - P2: Use "regenerate" audit action when updating existing token in generate_token, "create" only for new tokens - P2: Refresh token_hash (SHA-256) on upsert update and regenerate to keep DB hash consistent with new token value
| else: | ||
| token = APIToken.objects.create( | ||
| user=request.user, | ||
| token=api_key, | ||
| signature=sig, | ||
| label="Default", | ||
| expires_at=new_expiry, | ||
| ) | ||
| audit_action = "create" |
There was a problem hiding this comment.
MAX_KEYS_PER_USER limit not enforced on new-token creation path
The upsert logic correctly updates an existing "Default" token in-place, but when no "Default" token exists the else branch creates a brand-new APIToken record without first checking MAX_KEYS_PER_USER. A user who already holds the maximum number of non-Default keys can call this legacy endpoint to silently create an extra token that exceeds the per-user cap enforced by create_api_key.
create_api_key (lines 66–72 of the same file) shows the guard pattern:
max_keys = django_settings.MAX_KEYS_PER_USER
existing_count = APIToken.objects.filter(user=request.user).count()
if existing_count >= max_keys:
return Response({"message": f"Maximum {max_keys} API keys allowed per user."}, status=status.HTTP_429_TOO_MANY_REQUESTS)The same check should be added to generate_token before the else branch creates a new record.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/backend/core/routers/api_tokens/views.py
Line: 211-219
Comment:
**`MAX_KEYS_PER_USER` limit not enforced on new-token creation path**
The upsert logic correctly updates an existing `"Default"` token in-place, but when no `"Default"` token exists the `else` branch creates a brand-new `APIToken` record without first checking `MAX_KEYS_PER_USER`. A user who already holds the maximum number of non-Default keys can call this legacy endpoint to silently create an extra token that exceeds the per-user cap enforced by `create_api_key`.
`create_api_key` (lines 66–72 of the same file) shows the guard pattern:
```python
max_keys = django_settings.MAX_KEYS_PER_USER
existing_count = APIToken.objects.filter(user=request.user).count()
if existing_count >= max_keys:
return Response({"message": f"Maximum {max_keys} API keys allowed per user."}, status=status.HTTP_429_TOO_MANY_REQUESTS)
```
The same check should be added to `generate_token` before the `else` branch creates a new record.
How can I resolve this? If you propose a fix, please make it concise.| if token_value: | ||
| # Skip regeneration if the token hasn't changed | ||
| if existing_token and existing_token.token == token_value: | ||
| return | ||
| if existing_token: | ||
| existing_token.delete() | ||
|
|
||
| APIToken.objects.create(user=user, token=new_token, expires_at= now() + timedelta(days=90)) | ||
| api_key = generate_api_key() | ||
| token = APIToken.objects.create( | ||
| user=user, | ||
| token=api_key, | ||
| signature=generate_signature(api_key), | ||
| label="Default", | ||
| expires_at=now() + timedelta(days=django_settings.API_KEY_EXPIRY_DAYS), | ||
| ) | ||
| log_api_key_event( | ||
| request, action="create", key_id=token.id, | ||
| key_label="Default", key_masked=token.masked_token, | ||
| ) |
There was a problem hiding this comment.
MAX_KEYS_PER_USER limit not enforced in update_user_token
Before this PR update_user_token stored the raw frontend string — no real DB row was inserted. Now it calls APIToken.objects.create(...) (line 70), creating a real row that counts against the per-user key cap. However, no MAX_KEYS_PER_USER guard is applied before the create call, unlike create_api_key. A user who has reached the maximum via the KeyManagement UI can get an extra token simply by saving their profile page.
Consider adding the same count check:
max_keys = django_settings.MAX_KEYS_PER_USER
if APIToken.objects.filter(user=user).count() >= max_keys:
return # or surface an appropriate errorPrompt To Fix With AI
This is a comment left during a code review.
Path: backend/backend/core/views.py
Line: 62-80
Comment:
**`MAX_KEYS_PER_USER` limit not enforced in `update_user_token`**
Before this PR `update_user_token` stored the raw frontend string — no real DB row was inserted. Now it calls `APIToken.objects.create(...)` (line 70), creating a real row that counts against the per-user key cap. However, no `MAX_KEYS_PER_USER` guard is applied before the create call, unlike `create_api_key`. A user who has reached the maximum via the KeyManagement UI can get an extra token simply by saving their profile page.
Consider adding the same count check:
```python
max_keys = django_settings.MAX_KEYS_PER_USER
if APIToken.objects.filter(user=user).count() >= max_keys:
return # or surface an appropriate error
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Acknowledged — low risk in practice since both paths only create label="Default" tokens (one per user via upsert). The only edge case is if the Default token was manually deleted and the user already has MAX_KEYS_PER_USER keys from KeyManagement. Will add the guard in a follow-up if needed.
1 participant
What
vtk_prefix across all 3 token creation pathscreate_api_key()Why
user.py(get_or_create_valid_token) was generating tokens withuuid4().hex— plain hex, novtk_prefix, no labelviews.py(update_user_token) stored raw frontend token with no format enforcementapi_tokens/views.py(generate_tokenlegacy endpoint) generatedvtk_but never saved to DBHow
user.py: Replaceduuid4().hexwithgenerate_api_key(), addedlabel="Default",generate_signature(), andAPI_KEY_EXPIRY_DAYSviews.py: Now generates freshvtk_key viagenerate_api_key()instead of storing raw frontend valueapi_tokens/views.py: Legacy/token/generatenow creates a properAPITokenDB record with label, signature, and expiryCan this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
uuid4().hextokens already in DB will continue to work until they expirecreate_api_keyflow is completely unchangedDatabase Migrations
Env Config
API_KEY_EXPIRY_DAYSsetting (default: 90 days)Relevant Docs
Related Issues or PRs
Dependencies Versions
api_key_service.pyfunctionsNotes on Testing
get_or_create_valid_tokencreates tokens withvtk_prefixvtk_token saved to DBupdate_user_tokencreates proper APIToken recordcreate_api_keyflow (KeyManagement) still works unchangedScreenshots
N/A — backend-only changes
Checklist